gh-120417: Add #noqa to used imports in the stdlib #120421
Conversation
Tools such as ruff can ignore "imported but unused" warnings if a line ends with "# noqa". It avoids the temptation to remove an import which is used effectively.
vstinner
requested review from
rhettinger,
gpshead,
pganssle,
abalkin and
tiran
as code owners
vstinner
requested review from
berkerpeksag,
erlend-aasland,
brettcannon,
ericsnowcurrently,
ncoghlan and
warsaw
as code owners
Lib/collections/__init__.py
Outdated
| try: | ||
| from _collections import _deque_iterator | ||
| from _collections import _deque_iterator # noqa | ||
| except ImportError: | ||
| pass |
There was a problem hiding this comment.
test_collections passes for me if I remove this import. Does it need to stay? It was added in 52f96d3
There was a problem hiding this comment.
@erlend-aasland @kumaraditya303: Do you know/recall why this symbol is exposed?
There was a problem hiding this comment.
Oh, test_deque fails if the import is removed. I expected deque to be tested as part of test_collections, but apparently it has its own test file. Could you add a comment that it is (apparently) required to expose it in the collections module in order for deque iterators to be pickled?
======================================================================
ERROR: test_iterator_pickle (test.test_deque.TestBasic.test_iterator_pickle)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/alexw/dev/cpython/Lib/test/test_deque.py", line 640, in test_iterator_pickle
dump = pickle.dumps((itorg, orig), proto)
_pickle.PicklingError: Can't pickle <class 'collections._deque_iterator'>: attribute lookup _deque_iterator on collections failed
----------------------------------------------------------------------There was a problem hiding this comment.
Sure, I added a comment.
Lib/pydoc.py
Outdated
| from _pyrepl.pager import (get_pager, pipe_pager, | ||
| plain_pager, tempfile_pager, tty_pager, | ||
| plain) # noqa |
There was a problem hiding this comment.
Which is the import being flagged as unused here? Why does it need to stay?
There was a problem hiding this comment.
pydoc.plain() is an alias to _pyrepl.pager.plain(). It has to stay since it's part of pydoc API, and test_pydoc fails if you remove it.
# noqa only applies to the current line, no?
There was a problem hiding this comment.
pydoc.plain()is an alias to_pyrepl.pager.plain(). It has to stay since it's part of pydoc API, and test_pydoc fails if you remove it.
I see, thanks. Maybe add a comment explaining that to the code?
# noqaonly applies to the current line, no?
It depends on the tool, the rule and the AST node. For example, in this:
from collections import ( # noqa: F401
deque,
OrderedDict,
ChainMap,
)the single noqa comment will cause Ruff to ignore the three unused imports deque, OrderedDict and ChainMap, because they constitute a single ImportFrom node in the AST and the noqa comment is applied on the starting line number of the AST node.
There was a problem hiding this comment.
I moved the import to a separated statement and added a comment.
AlexWaygood
removed request for
brettcannon,
warsaw,
ncoghlan and
ericsnowcurrently
|
Instead of blanket everything But at least we could fix those when ruff enters the modern era and names things to be understandable by code maintainers. |
On a line like
So you suggest using |
it's on our to-do list :-) Edit: and I see Zanie already said that on the Ruff issue tracker ;) |
|
yep, always list the specific error(s) being ignored in the comment. |
I'm checking for unused imports for 5 years, and every year, I meet the same imports :-) Over time, I learnt which imports are done to populate an API, and which ones can be removed. You can try to remove them and see the test suite failing. |
|
I agree with all this, I just wondering how did you find them, because the amount of the work is large, and many cases are not trivial. Removing imports unlit the code breaks is risky, not all can be covered by tests. |
|
I modified the PR to use |
I also rely on code review to validate my work :-) |
There's a rule (PGH004 cpython/Tools/clinic/.ruff.toml Line 9 in ca5108a Shall we add it to the other one, https://github.com/python/cpython/blob/main/Lib/test/.ruff.toml ? And maybe add another
If we added F401 |
|
I created a second PR for the 7 files which requires tons of |
|
Merged, thanks for reviews! @hugovk: I'm interested by a CI checking for unused imports, it's appealing :-) |
|
For things that are re-exported for a public API, should add them to |
usually IMO, yes |
Tools such as ruff can ignore "imported but unused" warnings if a line ends with "# noqa: F401". It avoids the temptation to remove an import which is used effectively.
Tools such as ruff can ignore "imported but unused" warnings if a line ends with "# noqa: F401". It avoids the temptation to remove an import which is used effectively.
Tools such as ruff can ignore "imported but unused" warnings if a line ends with "# noqa: F401". It avoids the temptation to remove an import which is used effectively.
Tools such as ruff can ignore "imported but unused" warnings if a line ends with "# noqa". It avoids the temptation to remove an import which is used effectively.